Skip to content

fix sendFiles maxConcurrent - #1716

Open
erichanwang wants to merge 1 commit into
rocketride-org:developfrom
erichanwang:fix/sendfiles-max-concurrent
Open

fix sendFiles maxConcurrent#1716
erichanwang wants to merge 1 commit into
rocketride-org:developfrom
erichanwang:fix/sendfiles-max-concurrent

Conversation

@erichanwang

@erichanwang erichanwang commented Jul 28, 2026

Copy link
Copy Markdown

fixes #1381

changes:

  • add the documented optional maxConcurrent argument to TypeScript sendFiles
  • reject non-finite maxConcurrent values in both the CLI path and client API before calculating worker count
  • run uploads through a bounded worker loop instead of starting every upload at once
  • pass the validated CLI --max-concurrent option through to sendFiles
  • update the checked-in shell-api type signature and add focused concurrency regressions

CodeRabbit:

  • addressed the non-finite maxConcurrent client thread with a Number.isFinite guard and regression for Number.NaN
  • addressed the CLI thread by validating --max-concurrent before any connection or pipeline creation

verification:

  • git diff --check
  • pnpm install --frozen-lockfile (completed with existing missing rocketride bin symlink warnings)
  • pnpm exec jest tests/RocketRideClient.test.ts --runInBand -t "sendFiles concurrency" from packages/client-typescript -> 2 passed, 60 skipped; warned that the local RocketRide server was unavailable for unrelated integration tests
  • pnpm exec tsc -p tsconfig.json --noEmit from packages/client-typescript
  • pnpm exec tsc -p tsconfig.cli.json --noEmit from packages/client-typescript
  • pre-commit hook: gitleaks

notes:

  • Jest force-exit warning remains from the existing test harness after the focused tests complete.

Summary by CodeRabbit

  • New Features
    • File uploads now support a configurable maximum number of concurrent uploads (optional maxConcurrent, defaulting to 5).
    • The upload command-line option for concurrency is now enforced when sending files.
    • Upload results are returned in the same order as the selected files.
  • Bug Fixes
    • Invalid (non-finite) concurrency values are rejected with a clear error.
    • Individual file upload failures no longer halt the rest of the batch.
  • Tests
    • Added unit tests covering concurrency limiting and invalid maxConcurrent handling.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

sendFiles now accepts an optional concurrency limit, defaults to five uploads, and uses a worker pool to enforce it. The CLI forwards --max-concurrent, the public declaration is updated, and Jest coverage verifies bounded uploads and input validation.

Changes

Upload concurrency

Layer / File(s) Summary
Concurrency contract and CLI wiring
packages/shell-api/versions/v0.d.ts, packages/client-typescript/src/cli/rocketride.ts
The public sendFiles declaration accepts maxConcurrent, and the upload command validates and forwards its --max-concurrent value.
Bounded upload worker pool
packages/client-typescript/src/client/client.ts
sendFiles validates and defaults concurrency to five, processes files through a fixed worker pool, and preserves ordered results with isolated per-file errors.
Concurrency behavior tests
packages/client-typescript/tests/RocketRideClient.test.ts
Jest tests verify the configured maximum active uploads, one pipe invocation per input file, and rejection of non-finite limits.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RocketRideCLI
  participant RocketRideClient
  participant UploadPipe
  RocketRideCLI->>RocketRideClient: sendFiles(files, token, maxConcurrent)
  RocketRideClient->>UploadPipe: start up to maxConcurrent uploads
  UploadPipe-->>RocketRideClient: complete each upload
  RocketRideClient-->>RocketRideCLI: return ordered results
Loading

Suggested reviewers: jmaionchi, rod-christensen, stepmikhaylov

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements the documented maxConcurrent parameter, caps concurrency, and preserves the accepted 3-argument API.
Out of Scope Changes check ✅ Passed The changes stay focused on sendFiles concurrency, CLI wiring, type updates, and regression tests.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change by calling out the sendFiles maxConcurrent fix and is specific enough for history scanning.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor
🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete.

@erichanwang
erichanwang force-pushed the fix/sendfiles-max-concurrent branch from 1f11ac8 to af599b8 Compare July 30, 2026 20:18
@erichanwang
erichanwang marked this pull request as ready for review July 30, 2026 20:19
@erichanwang

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/client-typescript/src/client/client.ts`:
- Around line 1374-1378: Validate maxConcurrent with Number.isFinite before
calculating concurrency in sendFiles, rejecting non-finite values such as NaN
and Infinity. Preserve the existing minimum-concurrency behavior for finite
inputs and ensure invalid values cannot create a zero-worker upload path or
sparse results.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0e6d24a5-8888-4036-8b8c-ef9cfa90015b

📥 Commits

Reviewing files that changed from the base of the PR and between baea4ab and af599b8.

📒 Files selected for processing (4)
  • packages/client-typescript/src/cli/rocketride.ts
  • packages/client-typescript/src/client/client.ts
  • packages/client-typescript/tests/RocketRideClient.test.ts
  • packages/shell-api/versions/v0.d.ts

Comment thread packages/client-typescript/src/client/client.ts
@erichanwang
erichanwang force-pushed the fix/sendfiles-max-concurrent branch from af599b8 to 4799998 Compare July 30, 2026 20:27

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/client-typescript/src/cli/rocketride.ts`:
- Line 1455: Validate the max-concurrent CLI value before creating or connecting
the pipeline: parse it with Number(), reject non-finite values early, and return
through the existing CLI error path before invoking use() or sendFiles(). Keep
the client-side validation in sendFiles() as a defense in depth, and update the
sendFiles call in the surrounding command flow to use the validated value.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9309deda-449d-418a-8828-ee3220b99241

📥 Commits

Reviewing files that changed from the base of the PR and between af599b8 and 4799998.

📒 Files selected for processing (4)
  • packages/client-typescript/src/cli/rocketride.ts
  • packages/client-typescript/src/client/client.ts
  • packages/client-typescript/tests/RocketRideClient.test.ts
  • packages/shell-api/versions/v0.d.ts

Comment thread packages/client-typescript/src/cli/rocketride.ts
@erichanwang
erichanwang force-pushed the fix/sendfiles-max-concurrent branch from 4799998 to 16f2274 Compare July 30, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sendFiles() ignores its documented maxConcurrent argument

1 participant